Skip to content

[ENH] added afni 3dUnifize to utils #1906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 5, 2017
Merged

Conversation

salma1601
Copy link
Contributor

No description provided.

@salma1601 salma1601 changed the title added afni 3dUnifize to utils [ENH/WIP] added afni 3dUnifize to utils Mar 23, 2017
@codecov-io
Copy link

codecov-io commented Mar 24, 2017

Codecov Report

Merging #1906 into master will increase coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1906      +/-   ##
==========================================
+ Coverage   72.55%   72.93%   +0.37%     
==========================================
  Files        1064     1061       -3     
  Lines       54164    53800     -364     
  Branches     7809     7738      -71     
==========================================
- Hits        39301    39237      -64     
+ Misses      13645    13349     -296     
+ Partials     1218     1214       -4
Flag Coverage Δ
#smoketests 72.93% <100%> (+0.37%) ⬆️
#unittests 70.38% <100%> (+0.35%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/afni/__init__.py 100% <ø> (ø) ⬆️
nipype/interfaces/afni/utils.py 75.94% <100%> (+0.32%) ⬆️
nipype/pipeline/engine/workflows.py 76.41% <0%> (-0.09%) ⬇️
...nterfaces/freesurfer/tests/test_auto_MRIsExpand.py
nipype/sphinxext/plot_workflow.py
nipype/sphinxext/__init__.py
nipype/pipeline/engine/utils.py 80.43% <0%> (+0.19%) ⬆️
nipype/interfaces/freesurfer/utils.py 62.72% <0%> (+0.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd53dc...2ad6fe0. Read the comment docs.

(In other words, we do not recommend the use of 3dUniformize.)

For complete details, see the `3dUnifize Documentation.
<https://afni.nimh.nih.gov/pub../pub/dist/doc/program_help/3dUnifize.html>`_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://afni.nimh.nih.gov/pub/dist/doc/program_help/3dUnifize.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* Method: Obi-Wan's personal variant of Ziad's sneaky trick.
(If you want to know what his trick is, you'll have to ask him, or
read Obi-Wan's source code [which is a world of ecstasy and exaltation],
or just read all the way to the end of this help output.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider removing AFNI inside jokes. This doesn't really add to the documentation, and it exists in the linked page. (Also "all the way to the end" doesn't apply in this docstring.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

name_template='%s_unif',
desc='output image file name',
argstr='-prefix %s',
name_source='in_file')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this actually should be a file. My reading of the docs suggests that the actual output file would be named {out_file}{in_file} (or maybe {out_file}_{in_file}). I'd rewrite this as:

prefix = Str(argstr='-prefix %s', mandatory=True, desc='prefix of output dataset')

And then you'll need to add in Unifize:

    def _list_outputs(self):
        outputs = super(Unifize, self)._list_outputs()
        outputs['out_file'] = '{}{}'.format(self.inputs.prefix, self.inputs.in_file)
        return outputs

Or whatever the actual format of the out file would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually out_file is really a file, this is what AFNI expects even if it is called prefix.
I can not change the name to prefix, because AFNICommandInputSpec expects an input named out_file and adds it in any case.


class UnifizeOutputSpec(TraitedSpec):
out_file = File(desc='unifized file',
exists=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add output for scale factor dataset, if ssave is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

'from 1 mm.',
argstr='-Urad %s')
ssave = File(
name_template='%s_scale',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would drop the name_template here. There's no name_source, and users probably don't want this on by default.

And maybe call it scale_file for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

fix website typo
remove afni jokes
@effigies effigies dismissed their stale review May 4, 2017 21:46

Comments addressed.

@effigies
Copy link
Member

effigies commented May 4, 2017

This LGTM. I assume the reason I didn't merge is that it's still marked WIP. Did you have any further work you wanted to do here?

@salma1601
Copy link
Contributor Author

Did you have any further work you wanted to do here

No, sorry. Initially I wanted also to add 3dQwarp but I will do that in a separate PR because I will need more time.

@salma1601 salma1601 changed the title [ENH/WIP] added afni 3dUnifize to utils [ENH] added afni 3dUnifize to utils May 5, 2017
@effigies effigies merged commit 8d38c21 into nipy:master May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants